Skip to content

fix: step down when leader is removed from config#584

Open
Qian-Cheng-nju wants to merge 1 commit into
tikv:masterfrom
specula-org:fix/removed-leader-step-down
Open

fix: step down when leader is removed from config#584
Qian-Cheng-nju wants to merge 1 commit into
tikv:masterfrom
specula-org:fix/removed-leader-step-down

Conversation

@Qian-Cheng-nju

@Qian-Cheng-nju Qian-Cheng-nju commented Mar 25, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown

Hi, thanks for raft-rs!

I noticed an unimplemented code path in post_conf_change() flagged by TODO comments and put together a fix.

When a leader is removed from the voter set, post_conf_change() sets promotable=false but returns without stepping down. The removed leader continues sending heartbeats, which suppress elections on the remaining voters. This blocks the cluster from electing a new leader until the old one is shut down or partitioned.

The fix makes the removed leader transition to follower immediately. The included test (test_bug_removed_leader_does_not_step_down) demonstrates the issue with a 3-node cluster.

Summary by CodeRabbit

  • Bug Fixes

    • Leaders removed from the voter set now immediately step down to follower, stopping heartbeat emission and preventing stale leadership behavior.

  • Tests

    • New integration test verifies that a removed leader transitions to follower, becomes non-promotable, and ceases sending heartbeat messages after the removal.

@ti-chi-bot

ti-chi-bot Bot commented Mar 25, 2026

Copy link
Copy Markdown

Welcome @Qian-Cheng-nju! It looks like this is your first PR to tikv/raft-rs 🎉

@coderabbitai

coderabbitai Bot commented Mar 25, 2026 •
edited
Loading

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ebc2ba25-7f80-46f3-b83c-f67c6ca36d0c

📥 Commits

Reviewing files that changed from the base of the PR and between b13a837 and 186842e.

📒 Files selected for processing (2)
  • harness/tests/integration_cases/test_raft.rs
  • src/raft.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/raft.rs
  • harness/tests/integration_cases/test_raft.rs
📝 Walkthrough

Walkthrough

Leader nodes removed from the voter set now immediately transition to follower in post_conf_change. An integration test test_bug_removed_leader_does_not_step_down was added to verify the leader steps down, is non-promotable, and stops sending heartbeats after removal.

Changes

Cohort / File(s) Summary
Raft state change
src/raft.rs		 In post_conf_change, when the local node is leader but no longer a voter, call become_follower(self.term, INVALID_ID) to immediately step down (replaces prior comment-only branch).
Integration test
harness/tests/integration_cases/test_raft.rs		 Added test_bug_removed_leader_does_not_step_down to construct a 3-node cluster, remove the leader from voters, and assert it transitions to follower, is not promotable, and emits no heartbeats after stepping down.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I hopped where leaders used to stay,
A voter list took one away.
Down I bounced from crown to ground,
No more heartbeats, calm and sound.
Tests sing true — the fix is found! 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: making a leader step down when removed from the configuration, which is the core fix implemented in src/raft.rs and validated by the test.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Qian-Cheng-nju Qian-Cheng-nju force-pushed the fix/removed-leader-step-down branch from 28ea6b1 to b13a837 Compare March 25, 2026 07:58
@Qian-Cheng-nju
fix: step down when leader is removed from config
186842e 
Signed-off-by: Qian-Cheng-nju <Qian-Cheng-nju@users.noreply.github.com>
@Qian-Cheng-nju Qian-Cheng-nju force-pushed the fix/removed-leader-step-down branch from b13a837 to 186842e Compare March 25, 2026 08:21
@BusyJay

BusyJay commented Mar 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Thanks, how about porting etcd-io/raft#79 so that we will keep sync with upstream?

@Qian-Cheng-nju

Qian-Cheng-nju commented Mar 25, 2026

Copy link
Copy Markdown
Author

Thanks for the pointer! I've updated this PR to port etcd/raft#79 — the step-down behavior is now opt-in via a step_down_on_removal config option(default false) for backwards compatibility. Tests cover both the enabled and default cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

contribution dco-signoff: yes first-time-contributor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Qian-Cheng-nju @BusyJay